-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvement of context and quantization tables paragraphs #56
Conversation
ffv1.md
Outdated
|
||
PDF:$$context=Q_{0}[l-tl]+\left|Q_{0}\right|(Q_{1}[tl-t]+\left|Q_{1}\right|(Q_{2}[t-tr]+\left|Q_{2}\right|(Q_{3}[L-l]+\left|Q_{3}\right|Q_{4}[T-t])))$$ | ||
PDF:$$context=Q_{0}[l-tl]+Q_{1}[tl-t]+Q_{2}[t-tr]+Q_{3}[L-l]+Q_{4}[T-t]$$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had presumed that the pipes (|a|
) were to expressed absolute values, but in this edit they are simply removed. Does this semantic meaning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is abs() in the current RFC part, but I don't see what is absolute value of a table, and I don't see corresponding code in FFmpeg source code.
Note that the RFC version was containing multiplication too, and I don't see it too in FFmpeg source code.
See this snipset.
I am not sure of it, help would be appreciated for confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the abs()
in the RFC intended it to be a semantic equivalent to the pipes used in the LaTeX version. In the context of the plain text RFC, I thought abs()
would be more readable than |a|
. But I am not certain that the pipes in the LaTeX version indicate an absolute value but that is what the mathematical function section implies. Ping to @michaelni.
ffv1.md
Outdated
@@ -191,6 +191,8 @@ Background: a two's complement signed 16-bit signed integer was used for storing | |||
|
|||
## Context | |||
|
|||
For samples, compared to pixel X, named: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested rewording:
Samples, positioned relative to pixel
X
, are referenced to with the following abbreviations:
ffv1.md
Outdated
RFC:``` | ||
|
||
If the context is smaller than 0 then -context is used and the difference between the sample and its predicted value is encoded with a flipped sign. | ||
If context >= 0 then `context` is used and the difference between the sample and its predicted value is encoded as is, else `-context` is used and the difference between the sample and its predicted value is encoded with a flipped sign. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider tilde quotes on context >= 0
.
Do I understand correctly that if context is equal to -1
then since -1
is not greater than 0, then -(-1)
is used, ie 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Contexts can be positive only, the sign of stored context seems to be used only for saying that the encoded difference must be subtracted from the predicted value instead of added.
See this snipset
ffv1.md
Outdated
@@ -201,28 +203,41 @@ Background: a two's complement signed 16-bit signed integer was used for storing | |||
+---+---+---+---+ | |||
``` | |||
|
|||
The quantized sample differences L-l, l-tl, tl-t, t-T, t-tr are used as context: | |||
The quantized sample differences are used as context: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider These 5 quantized sample differences are used as context:
ffv1.md
Outdated
|
||
PDF:$$Q_{i}[a-b]=Table_{i}[(a-b)\&255]$$ | ||
PDF:$$Q_{j}[k]=quant_tables[i][j][k\&255]$$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaning of j
and k
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are arbitrary values in that case, as j
and k
are on the left and right, and they'll be in formulas when Q
is used.
In practice, j
is the context and k the pixel difference.
I don't think it is need to define them here, as we see that when Q
is used.
ffv1.md
Outdated
|
||
## Quantization table sets | ||
|
||
Several quantization table sets can be used in the same bitstream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be more specific? Is the number of quantization table sets per bitstream: >=0
, >=1
?
Is there a max limit to the count of quantization table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>=1
, it is quant_table_count
value. maybe we can add the constraint "must not be 0" in quant_table_count
definition.
FFmpeg has an hard-coded maximum of 8, but looks like this is not necessary in specs, so "unlimited" (as much as width or height i.e. at some point it is too much :) ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a positive integer
ffv1.md
Outdated
- For Cb and Cr planes, `quant_table_index [ 1 ]` index is used | ||
- For Alpha plane, `quant_table_index [ (version <= 3 || chroma_planes) ? 2 : 1 ]` index is used | ||
|
||
Background: in first implementations of FFV1 bitstream, the index for Cb and Cr planes was stored even if it is not used (chroma_planes set to 0), this index is kept for version <= 3 on order to keep compatibility with bitstreams in the wild. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on order
-> in order
ffv1.md
Outdated
```c | ||
function | type | ||
--------------------------------------------------------------|----- | ||
QuantizationTablePerContext(i, j, scale) { | | ||
v = 0 | | ||
for( k = 0; k < 128; ) { | | ||
len - 1 | sr | ||
for( a = 0; a < len; a++ ) { | | ||
len_minus1 | ur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider defining len_minus
as a temporary counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not a counter, it is a temporary value, so not defined. It is a bit explained in the intro ("storing the number of equal entries -1"), does it need a dedicated explicit definition? as it is temporary, I suggest not to do it.
f9378fe
to
040bb95
Compare
PR updated from comments from Dave, and including wording from #64 (typo fix is not backported as the list is completely removed, in order to avoid duplicated and potentially unsynchronized content) |
This contains quite a mixture of changes, some purely cosmetic some bitstream changes. These belong to several seperate commits |
ffv1.md
Outdated
|
||
## Quantization | ||
## Quantization tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section header is already used on line 1174. Duplicate section names should be avoided.
ffv1.md
Outdated
@@ -218,28 +220,41 @@ Background: a two's complement signed 16-bit signed integer was used for storing | |||
+---+---+---+---+ | |||
``` | |||
|
|||
The quantized sample differences L-l, l-tl, tl-t, t-T, t-tr are used as context: | |||
Relative to any sample `X`, the 5 quantized sample differences are used as context: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggesting: the following 5 quantized
Got it. Let's start with #68 then I'll update this PR. |
status? |
PR updated, @michaelni and @dericed please review again. I don't see multiplications, (or... What do I miss in the FFmpeg FFV1 decoder code?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but admit that I don't understand this equation well. I think I had misinterpreted the LaTeX when I added the parenthesis and abs.
Yes, i must have missed it but the |Q| stuff was meant as the number of elements / magnitude in the quantization table not as the absolute value of something |
No description provided.